Skip to content

Conversation

@KristofferC
Copy link
Member

@KristofferC KristofferC commented Nov 24, 2022

@KristofferC
Copy link
Member Author

Tweaked the printing for glue pkgs in precompilation:

image

@KristofferC
Copy link
Member Author

Status output:

image

Shows you glue packages and glue dependencies and which ones are loaded (in green).

@IanButterworth
Copy link
Member

Awesome. Should there be a + before the enabled glue packages or something, so that it's not just color

@KristofferC
Copy link
Member Author

Yeah, I can add that

callback only when glue packages are not supported
```julia
# This symbol is only defined on Julia versions that support glue packages
if isdefined(Base, :get_gluepkg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isdefined(Base, :get_gluepkg)
if !isdefined(Base, :get_gluepkg)

If I understand correctly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks :)

@halleysfifthinc
Copy link

Should this also include a REPL command to add glue packages, or is this experimental enough that the accessibility of a convenient REPL mode isn't desired?

Something like ] add --glue PkgName [Glue($PkgName)] [glue/PkgName] where ungiven optional glue package name and location arguments would be assumed to be "Glue$PkgName" and ProjectDir/glue/PkgName.jl or ProjectDir/glue/PkgName/PkgName.jl, respectively?

@KristofferC
Copy link
Member Author

KristofferC commented Nov 28, 2022

It could be added but it isn't rely necessary for the core functionality so maybe in another PR.

objects from a wide variety of different Julia packages.
Adding all those different Julia packages as dependencies
could be expensive since they would end up getting loaded even if they were never used.
Instead, these code required to plot objects for specific packages can be put into separate files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Instead, these code required to plot objects for specific packages can be put into separate files
Instead, the code required to plot objects for specific packages can be put into separate files

deps::Dict{VersionRange, Dict{String, UUID}}

# WeakCompat.toml
glue_compat::Dict{VersionRange, Dict{String, VersionSpec}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there will be new registry files written for these then? i.e. we'll need to update RegistryTools.jl to support these as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there already is a branch on LocalRegistry.jl supporting this.

# `julia` is always an implicit direct dependency
filter!(ctx.env.project.compat) do (name, _)
name == "julia" || name in keys(ctx.env.project.deps) || name in keys(ctx.env.project.extras)
name == "julia" || name in keys(ctx.env.project.deps) || name in keys(ctx.env.project.extras) || name in keys(ctx.env.project.gluedeps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer haskey(dict, name) than name in keys(dict)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just kept the existing way of writing it but that is better indeed.

update_manifest!(ctx.env, pkgs, deps_map, ctx.julia_version)

new = download_source(ctx)
fixup_glue!(ctx.env, pkgs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to call fixup_glue! from download_source? Or are there cases where we download, but don't want to fixup glue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Or have another function that calls both of them.

Before this PR we could create the manifest without even having to download the packages. Now, we need to look up the glue packages in the downloaded packages project file since that information is not stored in the registry.

An alternative to this is to not store the glue package mapping in the manifest at all and just look it up in the package's project file in code loading. But we have so far tried to avoid that opting for storing everything required for code loading in the manifest...

@KristofferC KristofferC changed the title add support for glue packages add support for package extensions Dec 3, 2022
Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just some minor formatting and wording etc.

KristofferC and others added 6 commits December 7, 2022 19:38
It is possible to think that `Registry` may be a standalone Pkg command with the previous docstring (e.g. a command that constructs or displays registry info).
`APIOptions` is defined as an alias for `Dict{Symbol, Any}`.
Consequently, the definition of a constructor for `APIOptions`
introduced a new constructor for Dict. This commit removes that
constructor.
`PackageToken` is a Union of multiple struct types. This commit removes
a constructor for `PackageToken` and replaces it with a function
`packagetoken`.

Furthermore, this commit removes `PackageIdentifier` as an alias for
String and replaces that with a struct type, which wraps a String
containing a package identifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants